datadog: sample_rate omitted by default #1
Conversation
| // Default: 1.0 | ||
| DatadogSampleRate float32 `json:"datadog-sample-rate"` | ||
| // Default: use a dynamic rate instead | ||
| DatadogSampleRate *float32 `json:"datadog-sample-rate",omitempty` |
There was a problem hiding this comment.
Is configuration ever printed/serialized after being parsed into this struct? If so, non-nil DatadogSampleRate would probably render as the pointer value, which is not what we want.
internal/ingress/controller/nginx.go
Outdated
| @@ -1084,37 +1074,65 @@ ratio = {{ .OtelSamplerRatio }} | |||
| parent_based = {{ .OtelSamplerParentBased }} | |||
| ` | |||
|
|
|||
| func datadogOpentracingCfg(cfg ngx_config.Configuration) (string, error) { | |||
| jsn := map[string]interface{}{ | |||
There was a problem hiding this comment.
I'd probably just call this m instead of jsn, especially since it's not json here, even if it becomes it later.
internal/ingress/controller/nginx.go
Outdated
| jsn["sample_rate"] = *cfg.DatadogSampleRate | ||
| } | ||
|
|
||
| jsnBytes, err := json.Marshal(jsn) |
There was a problem hiding this comment.
b or buf instead of jsnBytes. The name barely carries meaning because it's a forced placeholder with a 5 line lifecycle
internal/ingress/controller/nginx.go
Outdated
| func createOpentracingCfg(cfg ngx_config.Configuration) error { | ||
| var tmpl *template.Template | ||
| var fileContent string |
There was a problem hiding this comment.
Its final purpose is to be the contents of a file, but at this stage it's conf, config or at a stretch, configData
The part that makes it fileContent is the os.WriteFile at the end, not what it is until that point.
| @@ -1001,8 +994,7 @@ func NewDefault() Configuration { | |||
| DatadogEnvironment: "prod", | |||
| DatadogCollectorPort: 8126, | |||
| DatadogOperationNameOverride: "nginx.handle", | |||
| DatadogSampleRate: 1.0, | |||
| DatadogPrioritySampling: true, | |||
| DatadogSampleRate: nil, | |||
There was a problem hiding this comment.
This shouldn't be needed, except for consistency
There was a problem hiding this comment.
It's here for consistency.
internal/ingress/controller/nginx.go
Outdated
| } else { | ||
| tmpl, _ = template.New("empty").Parse("{}") | ||
| fileContent = "{}" |
There was a problem hiding this comment.
Would it make sense to just return here and skip creating a file, or log some warning? That might be handled somewhere else
There was a problem hiding this comment.
Nah, I'd prefer to leave it as it was. The current behavior is to produce a file containing {}, you can even see it on disk in the container.
|
Thanks for reviewing, @cgilmour. I'll type up a description for a PR to propose upstream. |
|
kubernetes#10151 proposed upstream. |
Here's what I plan to propose in a PR upstream.
It uses the magic float-1.0forsample_rateto mean "use priority sampling instead," and then changes the default value to be-1.0.It represents
DatadogSampleRateas a*float32instead offloat32. This way, the default value ofnilcan be interpreted to mean "no rate specified." The configuration is decoded by themapstructurepackage, which supports this style.In order to conditionally include
"sample_rate": ...in/etc/nginx/opentracing.json, I had to generalize the code that produces that file. I put the template-based implementation in a dedicated function, and that function is called for all tracers except Datadog. Datadog has its own function that uses theencoding/jsonpackage instead oftext/template.